Reviewed 2 of 3 files at r1.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)

multibody/parsing/detail_sdf_parser.cc, line 421 at r1 (raw file):

// This assumes that parent models will have their parsing start before child
// models!
sdf::InterfaceModelPtr ParseNestedInterfaceModel(

Just checking: this would work with files included by //world/include right? Not just //model/include

Reviewed 2 of 3 files at r1.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)

multibody/parsing/detail_sdf_parser.cc, line 523 at r1 (raw file):

  // WARNING: This means that failed parsing should invalidate the plant
  // forever?
  root->RegisterNestedModelParser(

this doesn't match the API proposed in gazebosim/sdf_tutorials#3, which has registerCustomModelParser(std::string, std::function<...>)

Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 523 at r1 (raw file):

Will update proposal to match this pseudo-code API.

multibody/parsing/detail_sdf_parser.cc, line 684 at r1 (raw file):

using VoidFunction = std::function<void()>;

VoidFunction AddNestedModelsFromSpecification(

Should mention (in parsing stages) how this will work, esp. with encapsulation etc.

Gave it an initial look. It looks like it implements the proposal in general terms. I'm not at my best today (as mentioned in some of the other comments) but it's gradually starting to click together, and I think (as you said, given the current constraints) that a final implementation looking something like this would work for me.

Reviewed 1 of 1 files at r2.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 507 at r2 (raw file):

  const Frame<double>& main_model_frame = get_model_frame(main_model_instance);
  RigidTransformd X_WMparsed =
      plant->GetDefaultFreeBodyPose(main_model_frame.body()) *

This might be a bug in drake's URDF parser (or I might not understand what's happening here), but the urdf parser never sets the default free body pose, and I believe it will create joints to the world while parsing the urdf (and that information would be lost here)

multibody/parsing/detail_sdf_parser.cc, line 518 at r2 (raw file):

  std::map<std::string, sdf::InterfaceModelPtr> interface_model_hierarchy;

  auto body_to_interface_frame = [&](const Body<double>& link) {

both of these lambdas (this and frame_to_interface_frame) are kinda melting my brain in terms of figuring out the intended effects. If this were a code review intended for merge I'd probably ask for some comments or something to walk someone with my "slept like shit last night" brain through what's going on. (in other words, it might be totally obvious to me tomorrow)

multibody/parsing/detail_sdf_parser.cc, line 524 at r2 (raw file):

    plant->SetDefaultPose(*link, X_WLdesired);
    // Register link as grounding frame.
    sdf::SemanticPose pose("", X_ML);

what's X_ML here?

multibody/parsing/detail_sdf_parser.cc, line 543 at r2 (raw file):

  };

  auto add_model = [&](ModelInstanceIndex model) {

I'm confused as to why this is a lambda. Isn't it just the body of the loop below? (I'm personally of the opinion that lambdas often hurt readability, but I know that's not a universally held belief)

multibody/parsing/detail_sdf_parser.cc, line 922 at r2 (raw file):

  nested_models_post_init();

  if (model.Static()) {

does the static field from the include element need to be handled for nested models?

Reviewable status: 8 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @sammy-tri, and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 523 at r1 (raw file):

Done.

multibody/parsing/detail_sdf_parser.cc, line 684 at r1 (raw file):

(pending parsing stages docs)

multibody/parsing/detail_sdf_parser.cc, line 507 at r2 (raw file):

OK I've added a note here.

However, thinking through it, I don't think there will be information loss:

• In URDF, all bodies should be connected; if there's a floating body, it should only be the first one.
• In MBP, if you SetDefaultFreeBodyPose on a floating body with other bodies attached to it via joints, all other bodies' positions don't matter

So yeah, should be good. Thanks for checking!

multibody/parsing/detail_sdf_parser.cc, line 518 at r2 (raw file):

OK Yeah, sorry about that. I've added brief comments, but am timing out atm.
For now, would like to keep them as lambdas, and then once real code comes through, will def. clean it up!

multibody/parsing/detail_sdf_parser.cc, line 524 at r2 (raw file):

Done. Oops!

multibody/parsing/detail_sdf_parser.cc, line 543 at r2 (raw file):

Done. (sorry, lazy Python habits!)

multibody/parsing/detail_sdf_parser.cc, line 922 at r2 (raw file):

Done. Oops!
(also made //include/static fail fast for custom models; libsdformat nested models should be fine.)

Reviewable status: 8 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @sammy-tri, and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 421 at r1 (raw file):

Done. (Added note)

Reviewed 1 of 1 files at r3.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 507 at r2 (raw file):

I don't think our URDF parser enforces any limits on the number of free bodies or their declaration order (I've got a WIP branch going on something unrelated which has multiple free bodies in a single URDF, though in that case the bodies are welded into an appropriate location by C++ code before the plant is finalized so it wouldn't actually matter what initial pose the parser picked)

Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @sammy-tri, and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 507 at r2 (raw file):

Er, are free bodies even valid in URDF?

Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @sammy-tri, and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 507 at r2 (raw file):

Er, are free bodies even valid in URDF?

Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @IanTheEngineer, and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 507 at r2 (raw file):

They're handled by drake's URDF parser in the manner I would personally expect (not the child of any joint, it's a free body). Looking through the URDF documentation, I don't see any clear indication of if it's valid to have a <link> with no corresponding <joint> (or to phrase it differently, a kinematic chain of joints which always ends in world). @IanTheEngineer do you have any ideas on what interpretations are found in other frameworks?

Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @IanTheEngineer, and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 507 at r2 (raw file):

also <joint type="floating"> is defined in the spec to create a free 6dof body though that leaves me wondering what this warning in the drake URDF parser means https://github.com/RobotLocomotion/drake/blob/master/multibody/parsing/detail_urdf_parser.cc#L441 (probably just that any other applicable properties of a floating joints (origin? limits? are being ignored)

Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @IanTheEngineer, and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 507 at r2 (raw file):
Ah, you're right; from the XML spec:
http://wiki.ros.org/urdf/XML/joint

So yeah, to me, the action item here is to ensure the default free body pose is set appropriately at this point, so that way reposturing can be done correctly.

It can either be done in the Drake URDF parser, or in this specific site (if setting in the URDF parser is inappropriate / confusing).

Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 507 at r2 (raw file):

My current thoughts (which may be wrong)... Drake's URDF parser doesn't parse the origin out of floating joints, though even if that issue is resolved some may not specify an origin (or not even specify a joint) or blindly use 0,0,0. We should probably add the missing feature to the URDF parser, but I don't think it makes any difference here.

What I'm having trouble with is deciding what would be the model frame (or canonical link) in the URDF case. AFAICT after parsing an urdf the following classes of links now exist in the world (and 0 to N of each type may be defined):

• Free/floating bodies -- assume the URDF parser has done the right thing with the default free body pose so far, so we just need to update that default to take include.pose_WM into account. This is your pose for the InterfaceLink

• Bodies with a joint directly to world (welded or otherwise) -- update the properties of the joint to take include.pose_WM into account. Possibly combine with the position of the joint to generate the pose for InterfaceLink.

• Bodies with joints to other bodies in the URDF. If we need a world pose for these on InterfaceLink, does that mean we need to resolve the first two cases and then traverse the kinematic chain to define the new InterfaceLink? I'm starting to wonder if there are cases where InterfaceLink::pose doesn't need to be populated (for drake anyway!) when MBP is always going to be doing FK based on the context to find it anyway (and the default depends on another joint).

Passing include.pose_WM into the URDF parser could potentially simplify the first two cases. It wouldn't affect the third case because the URDF parser doesn't resolve the world poses of those joints anyway.

As always, the above may involve confusion between the internals of the drake URDF parser + MBP and the wider world's expectation of what URDF means.

Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 507 at r2 (raw file):

My ideal is that we don't need to pass include.pose_WM to URDF; I think reposturing post-construction isn't that hard? (a bit messy, but meh?)

If I teach the URDF parser to use SetDefaultFreeBodyPose regardless of joint type (as is done by the SDFormat parser), then we're good, and we don't have to worry too much about the posturing side of things.
The only thing is the "model frame" for URDF; in this case, I'd just say, "take the first link", which ain't too hard?

Regarding how URDF does/doesn't handle floating body poses, I'm not sure about that... I think that's worth at least making an issue for it:
#13691

Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 684 at r1 (raw file):

Per this comment, may be simpler than initially thought:
gazebosim/sdf_tutorials#3 (review)

Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

multibody/parsing/detail_sdf_parser.cc, line 507 at r2 (raw file):

Calling SetDefaultFreeBodyPose on all bodies in an URDF sounds non-trivial (see my comments in #13691 about parent bodies for floating joints)

Dismissed @scpeters from a discussion.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @IanTheEngineer, and @sammy-tri)

multibody/parsing/detail_sdf_parser.cc, line 507 at r2 (raw file):

OK I think that makes sense, but I think it still should be manageable. (Goal here is to resolve this discussion for now ;)

I think this serves as a sufficient (albeit imperfect) indicator that the interface API is tractable and usable.
Resolving all comments. Please re-open them if you feel they're incomplete w.r.t. these considerations.

Related details (e.g. URDF and default free body pose, naming of NestedModel etc.) can be delegated to the respective issues / PR (drake issue, and proposal PR, respectively).

Closing PR for now and indicating that this is "resolved" for now.

Thank y'all!

Dismissed @azeey and @sammy-tri from 2 discussions.
Reviewable status: LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sammy-tri)